Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

docs(CONTRIBUTING): revert should be a valid type #12032

Closed
wants to merge 3 commits into from

Conversation

stevemao
Copy link
Contributor

@stevemao stevemao commented Jun 5, 2015

EG: 462f444

Review on Reviewable

@petebacondarwin
Copy link
Contributor

Well, strictly, revert is a modifier that you stick on the front of another commit's message.

@petebacondarwin
Copy link
Contributor

Can you describe it like that instead?

@stevemao
Copy link
Contributor Author

Since there is only one "modifier", should I just say revert?
I'm not 100% sure how you wanna write it but I'll push up something.

@stevemao
Copy link
Contributor Author

Also I'm maintaining the module conventional-changelog used by you guys.
what do you expect conventional-changelog to do to this? I would simply parse it as a type so it will look the same as other types.

## Bug Fixes
...

## Reverts
...

@petebacondarwin
Copy link
Contributor

Well at the moment, we are not actually using your package. Instead we use an script inside the angular.js project, changelog.js. And in this script you can see that we ignore all commits that do not start with fix, feat, perf or contain the word BREAKING. See https://github.com/angular/angular.js/blob/master/changelog.js#L194

The general idea, though, as far I am concerned, is that reverted commits should be removed from the changelog. So assuming that the original commit is also in the current release then we should simply ignore both commits. If it is reverting from a previous release, then I agree that we should have some kind of ## Reverts section.

@stevemao
Copy link
Contributor Author

Yup, that script is actually the very first version of conventional-changelog. You guys should definitely use the package once I release v0.1.x. It's much better :)
I agree with you. So far it's not smart enough to remove both commits within one release. Thanks for the feedback @petebacondarwin

Let me know what you think of the docs.

@@ -192,7 +192,7 @@ Each commit message consists of a **header**, a **body** and a **footer**. The
format that includes a **type**, a **scope** and a **subject**:

```
<type>(<scope>): <subject>
(revert: )<type>(<scope>): <subject>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the parentheses here on (revert: ) are because it is optional, right? But the parentheses on (<scope>) are because they are needed in the string. Perhaps we should just not include the revert bit in this and just explain about reverts as you do very clearly below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I was thinking the scope is actually optional. Maybe we should note what's optional what's not?

@stevemao
Copy link
Contributor Author

Things I found about the commit message guide is that it's actually not very strict:

  1. It doesn't say what's optional what's not.
  2. The exact format of a part.
    For example, it doesn't say how Breaking Changes should be written. The sentense should start with Breaking Change: and there are optional two newlines between Breaking Change: and the content of it.

@stevemao stevemao force-pushed the patch-5 branch 2 times, most recently from c7d18fa to b81d47f Compare July 1, 2015 00:00
@stevemao
Copy link
Contributor Author

stevemao commented Jul 1, 2015

This applies to the google doc you link to in the file. It gives more examples but still not very strict.

@stevemao
Copy link
Contributor Author

stevemao commented Jul 1, 2015

I added two commits just to support my points (Maybe they should be in different PR but they kinda related and I need your opinions). Let me know what you think. Thanks.

@stevemao
Copy link
Contributor Author

stevemao commented Jul 8, 2015

ping @petebacondarwin

@gkalpak
Copy link
Member

gkalpak commented Jul 8, 2015

Fun fact: According to the validate-commit-msg.js file, revert is considered a valid type since Nov 2012 (and there are tests that verify it - which probably no one runs 😃).

@stevemao
Copy link
Contributor Author

stevemao commented Jul 8, 2015

hm... lol. conventional-changelog treats revert as a type too (if it reverts a commit that's not in the current release) and it works well. There is no need to make an exceptional rule for it.

for validate-commit-msg.js I think it should verify the reverted commit actually exists.

@petebacondarwin
Copy link
Contributor

LGTM!

@stevemao
Copy link
Contributor Author

Thanks @petebacondarwin , @gkalpak anything you would like to add here?

@stevemao
Copy link
Contributor Author

Would be great if you guys could update the docs on google docs
https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y/edit#

Or, even better, create a git repo just for the conventions?

@stevemao
Copy link
Contributor Author

Thanks @petebacondarwin

@petebacondarwin
Copy link
Contributor

@stevemao I have also updated the google doc.

@stevemao
Copy link
Contributor Author

Nice, will check it out

stevemao added a commit to conventional-changelog/conventional-changelog that referenced this pull request Jul 14, 2015
stevemao added a commit to stevemao/angular that referenced this pull request Aug 4, 2015
mhevery pushed a commit to angular/angular that referenced this pull request Aug 4, 2015
mhevery pushed a commit to angular/angular that referenced this pull request Aug 4, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants